Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Canceling exceptions #87

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ilanbiala
Copy link
Contributor

This is a WIP of being able to cancel exceptions through the use of a String tag. @bunkat once this is ready to review with all the proper tests, I'll let you know. I'm creating the PR now so anyone can suggest changes if they come up with any ideas.

Start laying out code to cancel and exception
Add variables to track when an exception is being created.
@ilanbiala
Copy link
Contributor Author

@bunkat how can I keep track of when a time period is called for an exception and not a schedule? The issue I'm facing right now is that I would have to check in every function whether a variable that keeps track of whether an exception is being created is true or not to decide whether to set the value in the object of exception tags to exceptions.

@bunkat
Copy link
Owner

bunkat commented Feb 6, 2015

A time period is considered valid when it is valid for all schedules and invalid for all exceptions. There isn't really a concept of time period being called for an exception. Though maybe I'm not understanding the question.

If you are trying to tag exceptions so that you can find them later, you'll need to tag them as you add them (by updating the recur() functions or manually placing a tag in the appropriate exception schedule object). For example, if you wanted to tag an exception as a holiday you would tag it when you enter it and if you wanted to turn it off later, you would just search the exceptions for things with that tag and then remove them.

Add in exception tagging functionality
Add basic tests for exceptions
Add some functionality to .and() to accept an exception tag for a composite schedule
Add tagging functionality into .and() to enable tagging subsequent exceptions in a composite exception schedule.
Add remove exception functionality
Add tests for removing exceptions
@ilanbiala ilanbiala changed the title WIP: Canceling exceptions Feat: Canceling exceptions Feb 11, 2015
@ilanbiala
Copy link
Contributor Author

@bunkat I got all the tests I wrote passing and I even tested the code a little bit to see if it's pretty easy to work with. The functionality is all there and I didn't modify anything other than .recur() since that's what I use. Some of the tests that you or someone else wrote seem to be failing when I run it locally, and it seems that they are completely unrelated to mine. Do you know why some of the tests originally in Later aren't passing?

@ilanbiala
Copy link
Contributor Author

@bunkat if you take a look at my first commit in this PR, you'll see that I added an if statement that does nothing, and even then tests weren't passing. I'm pretty sure none of the tests started failing as a result of the changes I made.

@ilanbiala
Copy link
Contributor Author

@bunkat what should be done? Should we merge this in and then fix the failing tests in master?

@ilanbiala
Copy link
Contributor Author

By the way, since a composite exception schedule is created using .and() and not .and().except() I had to add a the composite tag functionality into .and() if you're wondering why I did that. Let me know if the sample code in the tests could be better and I'll try to come up with a better way.

@ilanbiala
Copy link
Contributor Author

@bunkat have you had a chance to look over this PR?

@bunkat
Copy link
Owner

bunkat commented Feb 12, 2015

Sorry, I'm quite busy at the moment and probably won't get a chance to look at this for a bit. Appreciate the PR and I'll get to it as soon as I can.

@ilanbiala
Copy link
Contributor Author

@bunkat if you have any questions about the PR, feel free to ask me so that we can get this merged in as quickly as possible for others to use.

@ilanbiala
Copy link
Contributor Author

@bunkat let me know if everything here is good so I can squash this PR down to 1 commit to make the commit log a little bit cleaner.

@ilanbiala
Copy link
Contributor Author

@bunkat is there any way you can take a look at this PR and test out the functionality I added in? I'd like to use this functionality for another project.

@bunkat
Copy link
Owner

bunkat commented Feb 16, 2015

You don't need to wait for me to be able to use this functionality. Just create a build out of your repository and go for it.

@ilanbiala
Copy link
Contributor Author

I'm referring to the server-side, because I'm looking to use this through the node package and not have to make lots of changes later because of monkey patching it now. Is there any issue with the code I added that prevents you from merging it in?

@ilanbiala
Copy link
Contributor Author

Any update on getting this PR merged in?

@ilanbiala
Copy link
Contributor Author

@bunkat do you have any questions about the functionality?

@bunkat
Copy link
Owner

bunkat commented Feb 24, 2015

Really sorry, I do try and keep up with things better but unfortunately I'm swamped right now and I'm not sure when I'm going to have a chance to get these PRs reviewed and merged in. I wouldn't wait on me, just use it as a local module instead of via npm. Once it gets merged it will be easy enough to move it over. I'll try and get to all these next month.

@ilanbiala
Copy link
Contributor Author

@bunkat just let me know when I should squash the commits so that you can merge.

@ilanbiala
Copy link
Contributor Author

@bunkat have you had a chance to take a look at this?

@ilanbiala
Copy link
Contributor Author

@bunkat can this be merged in and released as part of Later 1.2.0?

@ilanbiala
Copy link
Contributor Author

@bunkat everything coming along ok? It's been quite some time and I just wanted to see if you can still merge this PR.

@ilanbiala
Copy link
Contributor Author

@bunkat any issue with merging this?

@ilanbiala
Copy link
Contributor Author

@bunkat we should be testing against these new versions now that many people use them, can we merge this PR in?

@ilanbiala
Copy link
Contributor Author

@bunkat still waiting on this. Can you merge it in?

@niftylettuce
Copy link

Per my work with Bree, I have an updated fork of this package called @breejs/later. See https://github.com/breejs/later if you would like to file this PR there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants